-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(about-modal): moved to modal-box #5216
Conversation
Preview: https://patternfly-pr-5216.surge.sh A11y report: https://patternfly-pr-5216-a11y.surge.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, have a couple of questions for design.
src/patternfly/components/AboutModalBox/examples/AboutModalBox.md
Outdated
Show resolved
Hide resolved
@mattnolting can you rebase this against the v5 branch and update the base branch to v5? |
a628d20
to
271032e
Compare
@mcoker rebased and ready |
@@ -148,7 +128,6 @@ | |||
grid-template-columns: var(--pf-c-about-modal-box--lg--grid-template-columns); | |||
grid-template-rows: max-content max-content auto; | |||
max-width: var(--pf-c-about-modal-box--lg--MaxWidth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max-width: var(--pf-c-about-modal-box--lg--MaxWidth); |
looks like you can remove this, it's currently breaking the max-width
on lg screens. Although I think this begs the question of whether the about modal should manage its width at all, or if it should just consume the available space and be controlled by the modal. I'd lean toward the latter - wdyt @srambach @mceledonia @mcarrano?
fwiw if you fix this and let the max-width
apply, it looks like this in a modal with the modal width set to 50% of the viewport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I completely understand the question @mcoker . What's there now looks good to me. The model should have a max-width IMO. Maybe I'm struggling to differentiate between Modal and About modal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcoker updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcarrano sorry, it's definitely confusing. The about modal has a custom width and height set, since it is it's own implementation of a modal. With this update, I was proposing we remove the about modal's width, and just let it fill the size of whatever container it's in, which would mean the modal component is used to manage the about modal's width. The modal has width variations for small, medium, large, and a "custom" width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
b6ece2c
to
e1a966c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just one small update.
1e8c9b8
to
e16eef9
Compare
🎉 This PR is included in version 1.2.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 5.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #4526